Implement uring in AsyncWrite for fs::File
#7713
Conversation
ADD-SP
added
C-enhancement
|
This should be ready for merging i think, is there anything else blocking it? |
|
Sorry I've been too focused on #7696. I will try to review this PR too. |
tokio/src/fs/file.rs
Outdated
| #[derive(Debug)] | ||
| enum JoinHandleInner<T> { | ||
| Blocking(JoinHandle<T>), | ||
| #[cfg(all( | ||
| tokio_unstable, | ||
| feature = "io-uring", | ||
| feature = "rt", | ||
| feature = "fs", | ||
| target_os = "linux" | ||
| ))] | ||
| Async(BoxedOp<T>), |
There was a problem hiding this comment.
Here is an idea:
What if you perform the io-uring logic in a tokio::spawn task? That way, you can use JoinHandle in both cases.
I think it will simply code a lot.
There was a problem hiding this comment.
The Blocking(JoinHandle) is crate::blocking::JoinHandle not tokio::runtime::task::join::JoinHandle so some enum would be required anyway, and at that point might as well just have this kind of a boxed future
There was a problem hiding this comment.
That is a re-export of the same thing.
Line 9 in 4714ca1
There was a problem hiding this comment.
One reason that I suggest this is that the io-uring logic performs multiple writes in a loop to write the entire buffer, which requires the end-user to await flush or write on the file to make progress. However, the implementation we have today does not require the user to interact with the file to make progress - it happens in the background automatically.
There was a problem hiding this comment.
Ah got it, I'll change it.
Out of curiosity, general rule is that for a future to keep making progress, user needs to keep polling it. I feel like not having to do that specifically for fs::File is just an implementation detail, and users shouldn't rely on that always being the case. So i guess my question is how much is it worth preserving old behavior like this when implementing new features, if it comes at the cost of (small) performance overhead? relevant XKCD
There was a problem hiding this comment.
Had to adjust mocks to accommodate non-blocking spawning https://github.com/tokio-rs/tokio/actions/runs/19464918802/job/55697554687 b91836c - can you doublecheck im doing it right?
There was a problem hiding this comment.
This kind of thing is always a hard question. Though I would say that I'm pretty sure people rely on writes continuing even if the file is dropped, and it would be very hard to change that.
|
If someone can doublecheck that what I added to Also looks like GCP is breaking the CI :/ |
4 participants
Motivation
Currently IO operations for
fs::Fileis implemented usingspawn_blockingto perform IO, which is not great for an async runtime. This PR implements IO operations usingio_uring. Thefs::writefunction already usesio_uringfor async operations, and this PR extends the uring functionality intoimpl AsyncWrite for fs::File.For full discussion see here #7684 and subsequently on discord https://discord.com/channels/500028886025895936/810724255046172692/1427735341003051079
Solution
The
fs::Filestruct already implements an internal buffer when writing usingspawn_blocking, which is similar to whatio_uringrequires. Therefore inpoll_write, we can check ifio_uringis initialized and supported, and if not fall back to thespawn_blockingimplementation, without requiring any API changes.This PR only implements
AsyncWrite, and once this PR is accepted, I am happy to also open a PR forAsyncRead for Fileusing the same structure.